Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #1896: Thoroughly test ItemSelectionInputContainsAtLeastOneOfRuleClassifier #2073

Conversation

navneetsaluja
Copy link
Contributor

@navneetsaluja navneetsaluja commented Nov 2, 2020

Explanation

Fixes #1896

Tests for lowercase and uppercase inputs. (More test suggestions would be appreciated, this is just to ensure my skeleton is fine.)
(Any help on how to test that these test files are working properly would be appreciated as well)

Proof

Selection_018

Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
  • The PR follows the style guide.
  • The PR does not contain any unnecessary auto-generated code from Android Studio.
  • The PR is made from a branch that's not called "develop".
  • The PR is made from a branch that is up-to-date with "develop".
  • The PR's branch is based on "develop" and not on any other branch.
  • The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

@navneetsaluja
Copy link
Contributor Author

Will be fixing the lint test errors, could I get some leads on how to fix the Roboelectric tests? They are giving the following outputs:

Execution failed for task ':app:mergeDebugJavaResource'.
> Could not resolve all files for configuration ':app:debugRuntimeClasspath'.
   > Could not download byte-buddy.jar (net.bytebuddy:byte-buddy:1.6.11)
      > Could not get resource 'https://jcenter.bintray.com/net/bytebuddy/byte-buddy/1.6.11/byte-buddy-1.6.11.jar'.
         > Could not GET 'https://d29vzk4ow07wi7.cloudfront.net/(long URL)'
            > Connection reset
> Task :app:kaptGenerateStubsDebugKotlin FAILED
FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':app:kaptGenerateStubsDebugKotlin'.
> Could not resolve all artifacts for configuration ':app:debugCompileClasspath'.
   > Could not download mockito-core.jar (org.mockito:mockito-core:2.7.22)
      > Could not get resource 'https://jcenter.bintray.com/org/mockito/mockito-core/2.7.22/mockito-core-2.7.22.jar'.
         > Could not GET '(LONG URL)'
            > Connection reset

@anandwana001
Copy link
Contributor

anandwana001 commented Nov 2, 2020

@Arjupta @prayutsu would you like to help the contributor here with few initial thoughts on this.

@navneetsaluja Check our wiki guide here for ktlint issues - https://github.com/oppia/oppia-android/wiki/Ktlint-Guide

Copy link
Contributor

@Arjupta Arjupta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will get your failing lint checks done

@Arjupta
Copy link
Contributor

Arjupta commented Nov 3, 2020

Will be fixing the lint test errors, could I get some leads on how to fix the Roboelectric tests? They are giving the following outputs:

Execution failed for task ':app:mergeDebugJavaResource'.
> Could not resolve all files for configuration ':app:debugRuntimeClasspath'.
   > Could not download byte-buddy.jar (net.bytebuddy:byte-buddy:1.6.11)
      > Could not get resource 'https://jcenter.bintray.com/net/bytebuddy/byte-buddy/1.6.11/byte-buddy-1.6.11.jar'.
         > Could not GET 'https://d29vzk4ow07wi7.cloudfront.net/(long URL)'
            > Connection reset
> Task :app:kaptGenerateStubsDebugKotlin FAILED
FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':app:kaptGenerateStubsDebugKotlin'.
> Could not resolve all artifacts for configuration ':app:debugCompileClasspath'.
   > Could not download mockito-core.jar (org.mockito:mockito-core:2.7.22)
      > Could not get resource 'https://jcenter.bintray.com/org/mockito/mockito-core/2.7.22/mockito-core-2.7.22.jar'.
         > Could not GET '(LONG URL)'
            > Connection reset

Have you tried this after connecting to internet?

@navneetsaluja
Copy link
Contributor Author

@anandwana001 Thank you!

@Arjupta

Have you tried this after connecting to internet?

I'm not sure what you mean. All these tests were done on Github while pushing (here). Other than that, my local instance is always connected to the internet.

@Arjupta
Copy link
Contributor

Arjupta commented Nov 3, 2020

@anandwana001 Thank you!

@Arjupta

Have you tried this after connecting to internet?

I'm not sure what you mean. All these tests were done on Github while pushing (here). Other than that, my local instance is always connected to the internet.

I think you should run these tests once again. So just make a commit after correcting the lint error, it will automatically trigger the tests

@prayutsu
Copy link
Contributor

prayutsu commented Nov 3, 2020

Hi @navneetsaluja
I think this classifier is not about uppercase and lowercase strings. You should take a look at ItemSelectionContainsAtLeastOneOfRule and then try figuring out what you need to do.
ItemSelectionContainsAtLeastOneOfRule
Also, remove the TODO in the file ItemSelectionContainsAtLeastOneOfRule

@prayutsu
Copy link
Contributor

prayutsu commented Nov 3, 2020

Will be fixing the lint test errors, could I get some leads on how to fix the Roboelectric tests? They are giving the following outputs:

Execution failed for task ':app:mergeDebugJavaResource'.
> Could not resolve all files for configuration ':app:debugRuntimeClasspath'.
   > Could not download byte-buddy.jar (net.bytebuddy:byte-buddy:1.6.11)
      > Could not get resource 'https://jcenter.bintray.com/net/bytebuddy/byte-buddy/1.6.11/byte-buddy-1.6.11.jar'.
         > Could not GET 'https://d29vzk4ow07wi7.cloudfront.net/(long URL)'
            > Connection reset
> Task :app:kaptGenerateStubsDebugKotlin FAILED
FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':app:kaptGenerateStubsDebugKotlin'.
> Could not resolve all artifacts for configuration ':app:debugCompileClasspath'.
   > Could not download mockito-core.jar (org.mockito:mockito-core:2.7.22)
      > Could not get resource 'https://jcenter.bintray.com/org/mockito/mockito-core/2.7.22/mockito-core-2.7.22.jar'.
         > Could not GET '(LONG URL)'
            > Connection reset

These tests are giving this error because these are not relevant to this particular classifier, in this test we want to test if the inputs and answer have any common item or not. And that's what the name suggests, "contains at least one" (answer should contain at least one of the item in inputs).

@prayutsu
Copy link
Contributor

prayutsu commented Nov 3, 2020

@navneetsaluja You can do one more thing, you can add keywords Fixes #bugnum in the explanation section itself and not above the explanation section, then this would be more close to our conventional PR description

@navneetsaluja
Copy link
Contributor Author

@Arjupta Will commit again once I implement prayutsu's suggestions. Thank you.

@prayutsu I see, I already have three tests in mind, will try implementing them. Will edit the description of the PR as well to suit the conventions. Thank you for your input!

@rt4914
Copy link
Contributor

rt4914 commented Nov 3, 2020

Thanks a lot @prayutsu and @Arjupta for helping out new contributors. Thanks a lot.

@navneetsaluja
Copy link
Contributor Author

@prayutsu @Arjupta PTAL, checks passed, thanks.
Updated the tests too: they use a common Item Set as answer which is
["test1","test2","test3","test4","test5"]
I feel that the variables and the functions have bad names - not sure what to name them. Any more ideas for tests would also be helpful. Thanks!

@Arjupta
Copy link
Contributor

Arjupta commented Nov 4, 2020

@prayutsu @Arjupta PTAL, checks passed, thanks.
Updated the tests too: they use a common Item Set as answer which is
["test1","test2","test3","test4","test5"]
I feel that the variables and the functions have bad names - not sure what to name them. Any more ideas for tests would also be helpful. Thanks!

Changes I mentioned are done correctly. LGTM Thanks @navneetsaluja

@prayutsu
Copy link
Contributor

prayutsu commented Nov 4, 2020

@navneetsaluja I think variable names are fine, but I am not sure about the test case names, maybe @anandwana001 can suggest some good name.

@prayutsu
Copy link
Contributor

prayutsu commented Nov 4, 2020

@navneetsaluja It would be more close to our conventional title for the PRs if you add a colon after Fixes #bugnum like this Fixes #1896:
Also, it's good to have the PR title the same as the issue name.

@navneetsaluja navneetsaluja changed the title Fix #1896 created tests for ItemSelectionContainsAtLeastOneOfRule Fixes #1896: Thoroughly test ItemSelectionInputContainsAtLeastOneOfRuleClassifier Nov 4, 2020
@navneetsaluja
Copy link
Contributor Author

@prayutsu I see, will take care of it in future PRs. Thanks.

@navneetsaluja
Copy link
Contributor Author

@prayutsu new commit, PTAL :)

@Arjupta
Copy link
Contributor

Arjupta commented Nov 9, 2020

@Arjupta @prayutsu please resolve your comments, if you think the required changes are made.

@anandwana001 i think only the author of PR and people with write access can resolve them, as I am unable to find the resolve conversation button

@prayutsu
Copy link
Contributor

prayutsu commented Nov 9, 2020

@Arjupta @prayutsu please resolve your comments, if you think the required changes are made.

Actually, I am not getting the option to resolve comments.

@anandwana001
Copy link
Contributor

@Arjupta @prayutsu oh, ok, I got it. Thanks for update. The author will resolve it.

@rt4914 rt4914 assigned rt4914 and anandwana001 and unassigned navneetsaluja Nov 10, 2020
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks.

@rt4914 rt4914 removed their assignment Nov 10, 2020
Copy link
Contributor

@anandwana001 anandwana001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @navneetsaluja
nit change added

@navneetsaluja
Copy link
Contributor Author

navneetsaluja commented Nov 17, 2020

@/anandwana001(Not pinging because you've already been assigned)
Sorry for the late update, had real life commitments. Was thinking of using the createListOfSetsOfHtmlStrings defined in the InteractionObjectTestBuilder.kt but then it was rendering code a bit unreadable, so I just shifted my function to InteractionObjectTestBuilder.kt.
Other than that, it gives an App module test error here:
Caused by: org.gradle.process.internal.ExecException: Process 'Gradle Test Executor 15' finished with non-zero exit value 134 This problem might be caused by incorrect test process configuration.
; but on my local machine all the tests passed.

UPDATE: Checks passed now, I didn't change anything. When I wrote the original comment it was displaying the error above.

@Sarthak2601 Sarthak2601 removed their assignment Nov 17, 2020
Copy link
Contributor

@Sarthak2601 Sarthak2601 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

Copy link
Contributor

@anandwana001 anandwana001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thanks @navneetsaluja
Before the merge, could you please run all the test cases and put a screenshot in the description showing all tests are passing?

@rt4914 Adding you for further merge.

@navneetsaluja
Copy link
Contributor Author

@anandwana001 Added screenshot!

@rt4914 rt4914 merged commit 5724bca into oppia:develop Nov 18, 2020
@navneetsaluja navneetsaluja deleted the item-input-contains-at-least-one-of-rule-test branch November 18, 2020 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Thoroughly test ItemSelectionInputContainsAtLeastOneOfRuleClassifier
6 participants